-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cheatcodes): additional cheatcodes to aid in symbolic testing #8807
Conversation
1a2cc05
to
155c3c3
Compare
crates/cheatcodes/tests/random.rs
Outdated
} | ||
} | ||
|
||
contract CounterFreshStorageTest is DSTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests look good overall. I would also add several more tests:
- One that makes the storage fresh, then which reads/writes multiple storage values, and checks that we don't accidentally get aliasing between them somehow.
- One that makes the storage fresh, then reads two different values, and puts a precondition (assume) that they are distinct value (not equal to each other), and then does something with those values.
- One which reads a storage variable, but then bounds the result to something very small. This is to check if we can efficiently generate random numbers within a bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, will add such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test here that I think it covers first 2 bullets
foundry/crates/cheatcodes/tests/random.rs
Lines 152 to 165 in 46d2322
function test_fresh_storage_multiple_read_writes() public { | |
Counter counter = new Counter(); | |
vm.freshStorage(address(counter)); | |
uint256 slot1 = vm.randomUint(0, 100); | |
uint256 slot2 = vm.randomUint(0, 100); | |
require(slot1 != slot2, "random positions should be different"); | |
address alice = counter.owners(slot1); | |
address bob = counter.owners(slot2); | |
require(alice != bob, "random storage values should be different"); | |
counter.setOwner(slot1, bob); | |
counter.setOwner(slot2, alice); | |
assertEq(alice, counter.owners(slot2)); | |
assertEq(bob, counter.owners(slot1)); | |
} |
not clear though on how a test for 3rd one should look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, the third test maybe doesn't make sense, I agree. That one can probably be left for now, and if we encounter unexpected behavior with the tool we can report it and get a test added then.
The main concern is that a user may want to do this:
vm.freshStorage(address(counter));
vm.bound(counter.a(), 10, 20);
This basically would be saying: set the storage to all symbolic, but the value of a
should be between 10
and 20
. Currently, I assume this would cause a lot of reverts because the counter.a()
read will happen before the call to bound
.
I'm not sure there is anything you can do about this in Foundry, it should work fine in Kontrol. But instead, for Foundry, you would have to do:
vm.freshStorage(address(counter));
uint256 a = vm.freshUInt(256, 10, 20); // produces the fresh value _and_ bounds it
counter.setA(a);
Then we know that the storage is all symbolic, but the correct bounds exist on a
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is psuedocode, btw, just trying to brainstorm how to solve this issue of having a fully symbolic storage, but also allowing some bounds on specific values.
crates/cheatcodes/tests/random.rs
Outdated
contract DeterministicRandomnessTest is Test { | ||
|
||
function testDeterministicRandomUint() public { | ||
console.log(vm.randomUint()); | ||
console.log(vm.randomUint()); | ||
console.log(vm.randomUint()); | ||
} | ||
|
||
function testDeterministicRandomUintRange() public { | ||
uint256 min = 0; | ||
uint256 max = 1000000000; | ||
console.log(vm.randomUint(min, max)); | ||
console.log(vm.randomUint(min, max)); | ||
console.log(vm.randomUint(min, max)); | ||
} | ||
|
||
function testDeterministicRandomAddress() public { | ||
console.log(vm.randomAddress()); | ||
console.log(vm.randomAddress()); | ||
console.log(vm.randomAddress()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I would prefer the name freshUInt
, freshAddress
, for example, rather than random. Maybe they can be included in the forge-std, but then just simplify down to teh random
variants? The reason is because we want to use the same cheatcodes as Foundry directly, but for us they're not random, but symbolic. fresh
captures both random and symbolic as sub-categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the randomAddress
and uint are already added, going to work on the fresh*
cheatcodes and maybe look into aliasing existing
crates/cheatcodes/tests/random.rs
Outdated
require(counter.a() != 0); | ||
require(counter.b() != address(0)); | ||
require(counter.c() != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't see why these should hold.... As far as I can see, a
, b
, and c
should all be fresh, meaning they could be zero as well as any other number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's correct, they could be 0 though is unlikely. I will remove these assertions as they could fail the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the randomness though follows the fuzzing rules where you can specify a seed and have deterministic values. So maybe could leave the assertions but set a certain seed that will always generate non zero values for this sequence... will think if this is of any value for given test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ehildenb I removed the assertions and added a test using seed / producing deterministic values here:
foundry/crates/cheatcodes/tests/random.rs
Lines 198 to 216 in 46d2322
contract Counter { | |
uint256[] public a; | |
address[] public b; | |
int8[] public c; | |
bytes32[] public d; | |
} | |
contract CounterFreshStorageTest is DSTest { | |
Vm vm = Vm(HEVM_ADDRESS); | |
function test_fresh_storage_with_seed() public { | |
Counter counter = new Counter(); | |
vm.freshStorage(address(counter)); | |
assertEq(counter.a(11), 85286582241781868037363115933978803127245343755841464083427462398552335014708); | |
assertEq(counter.b(22), 0x939180Daa938F9e18Ff0E76c112D25107D358B02); | |
assertEq(counter.c(33), -104); | |
assertEq(counter.d(44), 0x6c178fa9c434f142df61a5355cc2b8d07be691b98dabf5b1a924f2bce97a19c7); | |
} | |
} |
Also ported Kontrol tests using same seed:
foundry/crates/cheatcodes/tests/random.rs
Lines 246 to 277 in 46d2322
contract SymbolicStore { | |
uint256 public testNumber = 1337; // slot 0 | |
constructor() {} | |
} | |
contract SymbolicStorageTest is DSTest { | |
Vm vm = Vm(HEVM_ADDRESS); | |
function test_SymbolicStorage() public { | |
uint256 slot = vm.randomUint(0, 100); | |
address addr = 0xEA674fdDe714fd979de3EdF0F56AA9716B898ec8; | |
vm.freshStorage(addr); | |
bytes32 value = vm.load(addr, bytes32(slot)); | |
assertEq(uint256(value), 85286582241781868037363115933978803127245343755841464083427462398552335014708); | |
// Load slot again and make sure we get same value. | |
bytes32 value1 = vm.load(addr, bytes32(slot)); | |
assertEq(uint256(value), uint256(value1)); | |
} | |
function test_SymbolicStorage1() public { | |
uint256 slot = vm.randomUint(0, 100); | |
SymbolicStore myStore = new SymbolicStore(); | |
vm.freshStorage(address(myStore)); | |
bytes32 value = vm.load(address(myStore), bytes32(uint256(slot))); | |
assertEq(uint256(value), 85286582241781868037363115933978803127245343755841464083427462398552335014708); | |
} | |
function testEmptyInitialStorage(uint256 slot) public { | |
bytes32 storage_value = vm.load(address(vm), bytes32(slot)); | |
assertEq(uint256(storage_value), 0); | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's cool.
0699b9d
to
a5bc7ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love all the additional tests, only have some questions about terminology
crates/cheatcodes/spec/src/vm.rs
Outdated
@@ -2303,6 +2312,14 @@ interface Vm { | |||
/// Unpauses collection of call traces. | |||
#[cheatcode(group = Utilities)] | |||
function resumeTracing() external view; | |||
|
|||
/// Random storage for given target address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean exactly, fresh
to me indicates that the account's storage is reset/cleared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like setArbitraryStorage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary sounds good to me too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh is just shorter/easier to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk, will stick with setArbitraryStorage
then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed, pls recheck
crates/cheatcodes/src/inspector.rs
Outdated
@@ -368,6 +371,9 @@ pub struct Cheatcodes { | |||
|
|||
/// Ignored traces. | |||
pub ignored_traces: IgnoredTraces, | |||
|
|||
/// Addresses that should have fresh storage generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we describe what fresh means, not obvious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, pls recheck (did a force push to keep PR clean)
crates/cheatcodes/tests/assume.rs
Outdated
use foundry_config::{Config, FuzzConfig}; | ||
use foundry_test_utils::{forgetest_init, str}; | ||
|
||
forgetest_init!(test_assume_no_revert, |prj, cmd| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like these tests to be all in forge
we're also seeing that there are 2 ways to write tests, one in testdata/... .sol and other in forgetest!, which i'm not too fond of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, will move them all under forge/tests/it
then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change, indeed they should stay in cheats testdata, got
Will follow up with a PR to cleanup force/cli/test_cmd.rs
as it is now a catch all thingy
crates/forge/tests/cli/test_cmd.rs
Outdated
[SOLC_VERSION] [ELAPSED] | ||
Compiler run successful! | ||
|
||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this changed? again the point of all these substitution is so that if the test output changes you can just run the test suite once with SNAPSHOTS=overwrite and it will be updated normally. now you have to do that and also re-add manually ...
since those are manual filters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, reverted, please check (got wrong the SNAPSHOTS=overwrite
usage but now clear, thank you)
af0aa16
to
10748dc
Compare
crates/cheatcodes/src/evm.rs
Outdated
let val = ccx.ecx.sload(target, slot.into())?; | ||
let mut val = ccx.ecx.sload(target, slot.into())?; | ||
// Generate random value if target should have arbitrary storage and storage slot untouched. | ||
if ccx.state.arbitrary_storage.contains(&target) && val.is_cold && val.data.is_zero() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's considered "untouched" here? eg in a single test calling the same contract two times from test scope would result in slot being cold on first call, and warm on second due to how our EVM works. is this expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that's the expected behavior, subsequent loads of same slot should return same value
// Load slot again and make sure we get same value. |
unless explicitly rewritten
// This should remain 0 if explicitly set. |
crates/cheatcodes/src/inspector.rs
Outdated
let key = try_or_return!(interpreter.stack().peek(0)); | ||
let target_address = interpreter.contract().target_address; | ||
if let Ok(value) = ecx.sload(target_address, key) { | ||
if value.is_cold && value.data.is_zero() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think this would never be cold because revm marks slot as warm during execution of opcode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though it looks like tests are passing? would need to look closer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be because we're reading the value instead of a key in interpreter.stack().peek(0)
. on step_end
stack already contains read value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah interpreter.current_opcode()
returns next opcode in step_end
, so it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point
I was thinking that we're entering ensure_arbitrary_storage
after revm executed the opcode, so it's my bad
though can we document what's happening here anyway? especially that this would happen before sload even though we are in _end
hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will add comment to make this clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klkvr please check 16f068f Had to accommodate copy from arbitrary storage scenario pointed here #8807 (comment) too, so a little bit bigger change, ptal. thanks!
impl Cheatcode for copyStorageCall { | ||
fn apply_stateful<DB: DatabaseExt>(&self, ccx: &mut CheatsCtxt<DB>) -> Result { | ||
let Self { from, to } = self; | ||
if let Ok(from_account) = ccx.load_account(*from) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't work for forking but Ig it's out of scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, wasn't discussed to work for forking but could be added later if needed
Another test I thought of: basically how do I would expect (and what we would have for symbolic execution) that if I copy a given contracts storage, I get the same storage as that contract. So if I set a storage to arbitrary, then copy it, all future reads from both storages will agree (even if reading from uninitialized slots). So a test could be:
|
yeah, this scenario wasn't covered, added support and suggested test here foundry/testdata/default/cheats/CopyStorage.t.sol Lines 100 to 157 in 16f068f
|
crates/forge/tests/it/cheats.rs
Outdated
@@ -26,6 +27,7 @@ async fn test_cheats_local(test_data: &ForgeTestData) { | |||
} | |||
|
|||
let mut config = test_data.config.clone(); | |||
config.fuzz.seed = Some(U256::from(100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be set in test_data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, not sure we should be adding a new TEST_DATA_WITH_SEED
(we don't have one for isolate either) but default tests shouldn't run with seed - since there's no option to set it inline I've added a test that runs contracts matching WithSeed
(excluded from defaults), hope this makes sense
pls check 7e69723
call.bytecode_address = *target; | ||
} else { | ||
// Check if we have a catch-all mock set for selector. | ||
if let Some(target) = mocks.get(&call.input.slice(..4)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can collapse into one get().or_else(|| )
, also this panics if call.input.len() < 4
, use call.input.get(..4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 7e69723
crates/cheatcodes/src/inspector.rs
Outdated
interpreter: &mut Interpreter, | ||
ecx: &mut EvmContext<DB>, | ||
) { | ||
if interpreter.current_opcode() == op::SLOAD { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should move the SLOAD
check outside into step_end
, and make it call one arbitrary_storage_end
to avoid callsite bloat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 7e69723
crates/cheatcodes/src/evm.rs
Outdated
let mut val = ccx.ecx.sload(target, slot.into())?; | ||
|
||
if val.is_cold && val.data.is_zero() { | ||
let rand_value = ccx.state.rng().gen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not gen unless either is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, changed in 7e69723
- separate cheatcodes tests with specific seed - better way to match mocked function - arbitrary_storage_end instead multiple calls - generate arbitrary value only when needed
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
async fn test_cheats_local(test_data: &ForgeTestData) { | ||
let mut filter = Filter::new(".*", ".*", &format!(".*cheats{RE_PATH_SEPARATOR}*")) | ||
.exclude_paths("Fork") | ||
.exclude_contracts("Isolated"); | ||
.exclude_contracts("(Isolated|WithSeed)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add it to if cfg!(feature = "isolate-by-default")
branch below too, tests with --features=isolate-by-default
are failing because we're overriding it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, missed running the test with feature set locally, added here 02086f9 (had to add mock function test too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to add mock function test too
hmm, wondering why would it break with --isolate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see, it's because we're ignoring bytecode_address
when handling isolated calls
TxKind::Call(call.target_address), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how to handle this now, probably not a big deal
could be solved by instead of altering bytecode_address
changing target address bytecode for duration of the call, though it would introduce new edge cases
let's keep it ignored for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if (self.arbitrary_storage.is_arbitrary(&interpreter.contract().target_address) || | ||
self.arbitrary_storage.is_copy(&interpreter.contract().target_address)) && | ||
interpreter.current_opcode() == op::SLOAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should just have the op::SLOAD
check and the others inside of the arbitrary_storage_end function; please be very mindful of what goes in step and step_end because these functions are called millions of times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok, will do a PR to change this, mind to explain the difference? (was thinking that is better to check if is arbitrary or copy of arbitrary storage first and only if one of them is true check if a SLOAD (which is more likely), as arbitrary and copy are not so commons)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are called on execution of every single opcode, meaning tens to hundreds of thousands per single execution of a test
we want them optimized for the common path of "do nothing" as most of these checks will be false most of the time; this means they should be always inlined and the checks should always be small in size so that these functions fit in as few cache lines as possible
because they are so hot this matters a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrt the checks order, the thinking is not correct because checking opcode is 1 or 2 instructions, while the others are hashmap lookups which are in the order of hundreds or thousands
in either case the most common and cheapest check is the SLOAD one, the rest should be outlined because most opcodes are not SLOAD
ideally hooks like these could be installed separately as custom opcodes, however we dont really have a framework for doing this currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, makes total sense, will follow up with a PR to get it inline
Re hashmap lookups - my logic was that it won't affect too much execution when cheatcodes not used (hence empty hashmap and no need to check the opcode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if better to have arbitrary storage as an option, made a draft PR here #8848
Motivation
cheatcodes
): support additional cheatcodes to aid in symbolic testing #4072 :copyStorage(address from, address to)
setArbitraryStorage(address target)
mockFunction(address callee, address target, bytes calldata data)
To be followed up with a PR for adding remaining
expect*Call
andfresh*
cheatcodes.Solution
setArbitraryStorage(address target)
:Utilities
grouptarget
address is added inarbitrary_storage
vecinspector.step_end
, generate random value if target address is marked as arbitrary storage, current opcode is SLOAD and storage slot untouched (coldensure_arbitrary_storage
fn added)vm.load
cheatcodecopyStorage(address from, address to)
:Utilities
groupmockFunction(address callee, address target, bytes calldata data)
:Evm
group (same with othermock*
cheatcodes)data
->target
address is added forcallee
(new hashmapmocked_functions
added)inspector.call
, mock target is looked up based on call's calldata (strict match first, if no match then matching by selector is attempted) and call's bytecode address is replaced with mock target, e.g. if usingmy_contract.mocked_args_function(456)
will be executed withmodel_contract
address bytecode .Catch-all function mocks can be set too, for example if using:
my_contract.mocked_args_function(ANY_VALUE)
will be executed withmodel_contract
address bytecode